Adopt the custom_injectors callback#13
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
chrismeyersfsu
force-pushed
the
AAP-35749-move-inject-credential
branch
7 times, most recently
from
6995214 to
3922ce7
Compare
|
@webknjaz Unsure what to do about the doc failure. |
src/awx_plugins/interfaces/_temporary_private_credential_api.py
Outdated
Show resolved
Hide resolved
@chrismeyersfsu one solution could be using this hack to produce artificial references to links somewhere in the source code: https://github.com/ansible/awx-plugins/pull/24/files#diff-85933aa74a2d66c3e4dcdf7a9ad8397f5a7971080d34ef1108296a7c6b69e7e3R209-R251. |
|
@chrismeyersfsu I'm trying something out. Hold on. We might be able to revert your commit with the ignore with what I'm trying to do. |
src/awx_plugins/interfaces/_temporary_private_credential_api.py
Outdated
Show resolved
Hide resolved
src/awx_plugins/interfaces/_temporary_private_credential_api.py
Outdated
Show resolved
Hide resolved
src/awx_plugins/interfaces/_temporary_private_credential_api.py
Outdated
Show resolved
Hide resolved
webknjaz
force-pushed
the
AAP-35749-move-inject-credential
branch
from
11dbd7a to
41a4361
Compare
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
@chrismeyersfsu everything works now. The branch could use a little cleanup, though.
chrismeyersfsu
force-pushed
the
AAP-35749-move-inject-credential
branch
2 times, most recently
from
86e3b6b to
5b4a72f
Compare
|
@webknjaz my cleaning up of the branch broke things or the fix didn't work. |
webknjaz
force-pushed
the
AAP-35749-move-inject-credential
branch
from
7f75fdd to
769791f
Compare
|
|
||
| from collections.abc import Callable | ||
|
|
||
| from awx_plugins.interfaces._temporary_private_credential_api import ( # noqa: WPS436, pylint: disable=C0301 |
There was a problem hiding this comment.
Urgh.. I missed this. Don't use cryptic numbers in pylint ignores. It's got proper human-readable names for rules...
There was a problem hiding this comment.
Ahhhh... I see. That's line-too-long.
There was a problem hiding this comment.
Anyway, the root cause can be fixed because the linting violation actually reveals a different problem — an absolute import. This should be using relative imports instead. And then, there wouldn't be a problem to suppress in the first place. We shouldn't hide such problems, but work to understand why they happen.
There was a problem hiding this comment.
If something needs to be ignored, there should be a code comment explaining why. Especially for uncommon flake8 rule codes. In case of pylint, they should be human-readable, which is often enough (but not always).
| @@ -0,0 +1,52 @@ | |||
| # pylint: disable=fixme | |||
There was a problem hiding this comment.
Also, global pylint ignores like this should be used as the last resort. They should be scoped to a single line or fixed instead.
Co-Authored-By: Sviatoslav Sydorenko <webknjaz@redhat.com>
webknjaz
force-pushed
the
AAP-35749-move-inject-credential
branch
from
769791f to
ea13414
Compare
webknjaz
force-pushed
the
AAP-35749-move-inject-credential
branch
from
ea13414 to
bc0e0de
Compare
|
@chrismeyersfsu post-merge instructions:
|
2 participants
No description provided.